HT-7: mail-behavior acceptance fixtures - #3
Conversation
Sends crafted emails at a live helpdesk mailbox and records the resulting conversations (via REST, embed=threads) as sanitized JSON fixtures — the future acceptance suite for Helpthread's mail engine. Five scenarios: new-conversation, reply-with-reference (captures the outbound agent reply's Message-ID over IMAP and replies against it), reply-subject-only, auto-submitted, same-subject-different-customer. Safety: every observation and the single mutation path (agent reply) are gated on a per-run subject marker — pre-existing conversations are untouchable. Deps (first in repo): nodemailer (MIT-0), imapflow (MIT), licenses verified at adoption per charter. Dry-run validated; live run + recorded fixtures follow in this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
Live run 23ad1ec4 against the production helpdesk recorded: - new-conversation: fresh email -> conversation with one customer thread - reply-subject-only: 'Re:' subject with no reference headers -> a SECOND conversation (no subject-based threading, confirmed by observation) - auto-submitted: Auto-Submitted mail -> conversation still created - same-subject-different-customer: identical subject, different sender -> separate conversation (no cross-customer merge) reply-with-reference timed out: the API-created agent reply thread exists in the helpdesk but no outbound email ever left it (confirmed absent from the customer mailbox 25+ min later) — investigation open; fixture intentionally not committed until the send path works. Harness fix: the error path wrote err.message unredacted (real addresses in timeout text); failures now pass through redact() like successes. The leaked local fixture was deleted before ever being committed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
The scenario's fixture records the crown-jewel behavior end to end:
outbound helpdesk reply carries Message-ID
<FS_reply-{threadId}-{token}@{domain}>, and a customer reply whose
In-Reply-To/References target it threads into the same conversation
(threads 3 -> 4, observed live).
Recorded deviations, kept as observations:
- REST-API-created 'message' threads exist in the DB but dispatch no
email; UI-sent replies do. (Production-relevant helpdesk finding.)
- Gmail SMTP rewrites plus-addressed From to the canonical account
address, so run 23ad1ec4 had ONE customer identity; scenario 5's
fixture relabeled honestly (same-customer variant, correction note),
and multi-customer runs need distinct sending identities.
Harness fix: waitForMessage matches on the unique subject marker only —
a plus-tagged To filter can never match what Gmail actually delivers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
Three more black-box fixtures completing the threading picture: - forged-reply-token: a tampered FS_reply token does NOT thread into the real conversation — it creates a new one. FreeScout validates the token, it doesn't just pattern-match the format. - token-authority: a VALID token with an unrelated subject threads into the token's conversation anyway. The signed token is the sole threading authority; subject is irrelevant. - html-body: an inbound <script> tag is stored VERBATIM in the thread body and returned raw by the API. Sanitization is entirely the reader's responsibility — a stored-XSS surface our engine must own. Redaction hardened (real leaks found in the first pass, now fixed and prevented going forward): domain scrub (reply-token Message-ID domain, avatar URL host, bare domain), structural person-identity redaction (name/avatar on any person-shaped object), and explicit display-name scrubbing for names embedded in audit-log thread bodies. All eight committed fixtures verified free of real names, emails, domains, and avatar hashes. send.mjs gained html support for the sanitization probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds observed mail fixtures documenting conversation creation, reply threading, token authority and forgery handling, automated messages, subject-only behavior, same-subject messages, and HTML body capture. ChangesObserved mail behavior fixtures
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
fixtures/mail/observed/html-body.json (1)
75-75: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDo not turn raw script persistence into a passing security contract.
If acceptance tests consume this fixture as expected output, the preserved
<script>can normalize stored-XSS behavior. Keep it as an observation, but add a structured assertion that rendering escapes/removes scripts, or explicitly mark this fixture as a known security regression.Also applies to: 112-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fixtures/mail/observed/html-body.json` at line 75, Keep the raw script tag in the observed fixture as input evidence, but update the acceptance tests consuming this fixture to assert that rendered output escapes or removes script content. Alternatively, explicitly flag the fixture through the relevant test metadata as a known security regression; use the fixture’s HTML-body observation and its associated rendering/assertion helpers to implement this.fixtures/mail/observed/reply-with-reference.json (1)
269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepresent outbound-delivery observations as structured data.
The PR objective includes that API-created replies did not dispatch email, but this fact is currently only prose. Add a machine-readable field such as
apiReplyEmailDelivered: falsewith the observation window, while retaining the note for context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fixtures/mail/observed/reply-with-reference.json` around lines 269 - 273, Add a structured observation field alongside the prose entries in the notes fixture, such as apiReplyEmailDelivered: false and an observation window indicating the 25+ minute delay; retain the existing explanatory note for context.fixtures/harness/send.mjs (1)
14-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd SMTP connection/socket timeouts.
No timeout options on the transporter — a stalled TLS handshake or slow server can hang indefinitely, well past the 240s scenario deadlines the harness otherwise enforces.
♻️ Proposed fix
transporter = nodemailer.createTransport({ host: 'smtp.gmail.com', port: 465, secure: true, + connectionTimeout: 15000, + greetingTimeout: 15000, + socketTimeout: 30000, auth: { user: env.smtpUser, pass: env.smtpPass, }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fixtures/harness/send.mjs` around lines 14 - 27, Configure connection and socket timeouts in the nodemailer transporter created by getTransporter, using bounded values shorter than the harness’s 240-second scenario deadline so stalled TLS handshakes or SMTP activity fail promptly.fixtures/harness/api.mjs (1)
18-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to
apiFetch.No timeout on
fetch()— a stalled connection to FreeScout would hang indefinitely, bypassing the deadline checks in every poll loop that calls this.♻️ Proposed fix
async function apiFetch(path, init = {}) { const env = loadEnv(); const url = `${env.fsBaseUrl}${path}`; - const res = await fetch(url, { - ...init, - headers: { ...authHeaders(), ...(init.headers ?? {}) }, - }); + const res = await fetch(url, { + ...init, + headers: { ...authHeaders(), ...(init.headers ?? {}) }, + signal: AbortSignal.timeout(15000), + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fixtures/harness/api.mjs` around lines 18 - 31, Add an AbortController-based timeout inside apiFetch, using a bounded duration from configuration or a sensible default, and pass its signal to fetch alongside the existing request options. Ensure the timeout is cleared after completion and timeout failures surface as descriptive errors while preserving existing response handling.fixtures/harness/inbox.mjs (1)
100-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
toAddressparameter insearchMailbox.
toAddressis threaded through both call sites but never used insidesearchMailbox— onlysubjectContainsfilters the search. The comment above explains why recipient filtering was dropped, but the plumbing was left behind, which is confusing for future readers.Also applies to: 141-141, 148-148, 152-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fixtures/harness/inbox.mjs` around lines 100 - 117, Remove the unused toAddress parameter from searchMailbox and update every call site, including the references around the noted lines, to pass only the client, path, and subjectContains arguments. Preserve the existing subject-only search behavior and adjust any related comments or documentation that mention the removed parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fixtures/harness/env.mjs`:
- Around line 64-71: Validate HARNESS_FS_USER_ID in the config construction
before assigning it to fsUserId; reject non-numeric or otherwise invalid values
with a clear configuration error instead of allowing NaN. Update the fsUserId
handling near OPTIONAL_DEFAULTS.HARNESS_FS_USER_ID while preserving the numeric
default behavior.
In `@fixtures/harness/inbox.mjs`:
- Around line 145-161: Move the openClient() call inside the retry loop’s
try/catch structure, using a client variable initialized before the block so
logout only runs when a connection was successfully established. Catch transient
connection or search errors, allow the loop to continue until the deadline, and
retain the existing finally-based logout cleanup for connected clients.
In `@fixtures/harness/README.md`:
- Around line 80-89: Update the “Safety rules” section in README.md to state
that isolation is enforced by the run-scoped [HT7-<runId>-...] marker, not
always by each scenarioId. Explicitly document that
same-subject-different-customer.json intentionally reuses the prior
new-conversation subject and may observe both same-run matching conversations,
while cross-run conversations remain excluded; revise the marker-gated mutation
wording consistently.
In `@fixtures/harness/redact.mjs`:
- Around line 125-137: The walk function’s person-object handling still passes
email values through generic string redaction, allowing unrelated domains to
remain unchanged. Update the isPerson branch to replace every non-null email
using the stable role-based IDENTITY_FIELD_REPLACERS placeholder logic, while
preserving null values and the existing object structure.
- Around line 82-97: The buildDomainRules function only redacts the domain of
FS_reply-* Message-IDs, leaving reusable capability tokens exposed. Add a rule
that detects and replaces the complete FS_reply-* token with a deterministic
fake while preserving the expected format, applying it before the generic domain
rule; alternatively invalidate or rotate the token in the fixture.
In `@fixtures/harness/run.mjs`:
- Around line 175-215: Scenario failures are recorded but do not affect the
process exit status. In the scenario loop within main(), set process.exitCode =
1 whenever outcome is 'timeout-or-error', while continuing to write the failure
fixture and process subsequent scenarios.
- Around line 195-209: Use the sanitized error text from the redacted `payload`
when reporting failures in the `run` harness flow. Update the `console.error`
call in the timeout-or-error handling block to log `payload.error` rather than
the raw `err.message`, while preserving the existing scenario ID and duration
context.
- Around line 179-208: Update the harness redaction flow in the success and
failure paths to collect the known identity names before calling redact. Pass
those names through the identityNames option in both redaction calls, including
the calls around the recorded payload and timeout-or-error payload, so names in
notes and error text are sanitized.
- Around line 33-43: Update parseArgs to require a non-empty value after --only,
throwing a clear error when it is missing, and reject any unrecognized options
instead of silently ignoring them. Preserve valid --only and --dry-run handling,
and ensure callers handle argument-parse errors without falling back to running
all scenarios.
In `@fixtures/harness/scenarios.mjs`:
- Around line 96-104: Guard the reply construction in the scenario using
outboundAgentReply.messageId before assigning the In-Reply-To header. Omit
In-Reply-To or use a valid fallback when the value is null, while preserving the
existing References behavior and reply send flow.
- Around line 218-266: Make runSameSubjectDifferentCustomer explicitly require
the new-conversation scenario’s prior conversation before sending. When before
is empty or no priorConversationId is found, return a clear skipped/invalid
result (or fail the scenario) explaining that it must run after
new-conversation, and do not classify the new message as own-conversation.
Update the observed data and notes accordingly so --only execution is
unambiguous.
In `@fixtures/mail/observed/same-subject-different-customer.json`:
- Around line 3-4: The fixture incorrectly presents same-customer messages as
cross-customer coverage. Update the fixture data and expectations to use a
genuinely different customer/sending identity, or rename the scenario and
objective to explicitly describe same-customer identical-subject behavior;
ensure the related cases around the corresponding expectation entries are
consistent.
In `@fixtures/mail/observed/token-authority.json`:
- Around line 7-10: The token-authority fixture contains an invalid,
non-reproducible run ID. Regenerate fixtures/observations using the current
runner so all related subject and header markers use a valid eight-character
hexadecimal ID, or explicitly mark the artifact as hand-curated if it must
retain its current values.
---
Nitpick comments:
In `@fixtures/harness/api.mjs`:
- Around line 18-31: Add an AbortController-based timeout inside apiFetch, using
a bounded duration from configuration or a sensible default, and pass its signal
to fetch alongside the existing request options. Ensure the timeout is cleared
after completion and timeout failures surface as descriptive errors while
preserving existing response handling.
In `@fixtures/harness/inbox.mjs`:
- Around line 100-117: Remove the unused toAddress parameter from searchMailbox
and update every call site, including the references around the noted lines, to
pass only the client, path, and subjectContains arguments. Preserve the existing
subject-only search behavior and adjust any related comments or documentation
that mention the removed parameter.
In `@fixtures/harness/send.mjs`:
- Around line 14-27: Configure connection and socket timeouts in the nodemailer
transporter created by getTransporter, using bounded values shorter than the
harness’s 240-second scenario deadline so stalled TLS handshakes or SMTP
activity fail promptly.
In `@fixtures/mail/observed/html-body.json`:
- Line 75: Keep the raw script tag in the observed fixture as input evidence,
but update the acceptance tests consuming this fixture to assert that rendered
output escapes or removes script content. Alternatively, explicitly flag the
fixture through the relevant test metadata as a known security regression; use
the fixture’s HTML-body observation and its associated rendering/assertion
helpers to implement this.
In `@fixtures/mail/observed/reply-with-reference.json`:
- Around line 269-273: Add a structured observation field alongside the prose
entries in the notes fixture, such as apiReplyEmailDelivered: false and an
observation window indicating the 25+ minute delay; retain the existing
explanatory note for context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8eb8678-f66d-496b-85ca-d69ee1a977fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
fixtures/harness/README.mdfixtures/harness/api.mjsfixtures/harness/env.mjsfixtures/harness/inbox.mjsfixtures/harness/redact.mjsfixtures/harness/run.mjsfixtures/harness/scenarios.mjsfixtures/harness/send.mjsfixtures/mail/observed/auto-submitted.jsonfixtures/mail/observed/forged-reply-token.jsonfixtures/mail/observed/html-body.jsonfixtures/mail/observed/new-conversation.jsonfixtures/mail/observed/reply-subject-only.jsonfixtures/mail/observed/reply-with-reference.jsonfixtures/mail/observed/same-subject-different-customer.jsonfixtures/mail/observed/token-authority.jsonpackage.json
| // Error messages can embed real addresses (e.g. waitForMessage timeouts) — | ||
| // the failure path gets redacted exactly like the success path. | ||
| payload = redact( | ||
| { | ||
| scenario: scenario.id, | ||
| title: scenario.title, | ||
| expectation: scenario.expectation, | ||
| runId, | ||
| recordedAt: new Date().toISOString(), | ||
| outcome: 'timeout-or-error', | ||
| error: err.message, | ||
| }, | ||
| { smtpUser: envConfig.smtpUser, helpdeskAddr: envConfig.helpdeskAddr }, | ||
| ); | ||
| console.error(`[harness] ${scenario.id} FAILED after ${Date.now() - startedAt}ms: ${err.message}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the raw failure message.
The code correctly redacts payload.error, but Line 209 prints the original err.message; the preceding comment already acknowledges that timeout errors may contain real addresses. This leaks PII into CI or terminal logs even when the fixture is sanitized.
Log the sanitized error text instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fixtures/harness/run.mjs` around lines 195 - 209, Use the sanitized error
text from the redacted `payload` when reporting failures in the `run` harness
flow. Update the `console.error` call in the timeout-or-error handling block to
log `payload.error` rather than the raw `err.message`, while preserving the
existing scenario ID and duration context.
| "title": "Same customer, identical subject, no reference headers (intended different-customer — see correction)", | ||
| "expectation": "A second email reusing the exact subject of an existing conversation, with no reference headers, may merge or create its own conversation — observed, not assumed.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not claim cross-customer coverage from this fixture.
Both conversations belong to customer id 3, and the correction explicitly says Gmail normalized the plus-address to the same canonical sender. This proves same-customer identical-subject behavior, not the PR objective’s cross-customer collision behavior.
Use a real second sending identity, or narrow the scenario title and objective to same-customer subject collisions.
Also applies to: 368-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fixtures/mail/observed/same-subject-different-customer.json` around lines 3 -
4, The fixture incorrectly presents same-customer messages as cross-customer
coverage. Update the fixture data and expectations to use a genuinely different
customer/sending identity, or rename the scenario and objective to explicitly
describe same-customer identical-subject behavior; ensure the related cases
around the corresponding expectation entries are consistent.
Security (CodeRabbit 🔒 Major): reply tokens are a routing capability, not just an id — redact.mjs now replaces the whole FS_reply-* token with a deterministic fake (was scrubbing only the domain), and person-email fields on unexpected domains get a role placeholder instead of passing through. Committed fixtures re-scrubbed: real/forged/reference tokens are now distinct, consistent fakes (1111…/3333…/2222…); no real token hex remains anywhere. Robustness (CodeRabbit 🩺/🎯): waitForMessage now retries transient IMAP connect/auth failures instead of aborting the whole poll (openClient moved inside try, optional-chained logout); HARNESS_FS_USER_ID is validated as a positive integer instead of silently becoming NaN. Docs: README safety rule reworded to describe run-scoped marker isolation accurately, including the intended same-run subject-collision exception. Dry-run re-verified; all fixtures valid JSON. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fixtures/harness/redact.mjs`:
- Around line 153-158: The person email validation in the redaction branch
should accept both bare example.test addresses and subdomain forms. Update the
suffix check in the isPerson email handling to recognize values ending with
“@example.test” or “.example.test”, preserving the scrubbed deterministic
address instead of replacing it with person@example.test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 660f1e43-c3f4-417f-8e6b-04f6657e2658
📒 Files selected for processing (7)
fixtures/harness/README.mdfixtures/harness/env.mjsfixtures/harness/inbox.mjsfixtures/harness/redact.mjsfixtures/mail/observed/forged-reply-token.jsonfixtures/mail/observed/reply-with-reference.jsonfixtures/mail/observed/token-authority.json
✅ Files skipped from review due to trivial changes (2)
- fixtures/mail/observed/forged-reply-token.json
- fixtures/harness/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- fixtures/mail/observed/token-authority.json
- fixtures/harness/env.mjs
- fixtures/harness/inbox.mjs
…ssion
Fixes the incremental findings CodeRabbit surfaced on run.mjs/scenarios.mjs
(files it reached in round 2):
- redact.mjs: fix a regression from the prior fix — the person-email guard
used .endsWith('.example.test'), which misses bare 'support@example.test'
forms and would over-scrub good customer tags on future runs. Now matches
any *example.test address. (Committed fixtures were never affected — they
were reset+token-sed'd, not re-run through the buggy guard.)
- run.mjs: log the REDACTED error, not raw err.message (PII could reach
terminal/CI logs); set process.exitCode=1 on any scenario timeout/error so
CI can't read a failed run as success; validate --only's value and reject
unknown options (a typo in live mode could otherwise send every probe).
- run.mjs + env.mjs: thread identityNames through both redact calls via a new
optional HARNESS_IDENTITY_NAMES env var, so free-text names in notes/audit
bodies are scrubbed on future runs (documented in README).
- scenarios.mjs: guard a null outbound messageId from becoming the literal
'null' In-Reply-To header; fail fast if same-subject-different-customer is
run standalone via --only (depends on new-conversation running first).
All verified: syntax, dry-run, suffix-fix, name-scrub, and both arg guards.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
…istributed repo The FreeScout-observation harness served its purpose (generated these fixtures, surfaced the behavioral facts, found a prod send-bug) but doesn't belong in a distributed product: it observes FreeScout specifically and serves no Helpthread deployer. It now lives in the private resonant-help repo (tools/mail-behavior-harness/). What ships here: the mail-behavior acceptance fixtures, reframed as Helpthread's own acceptance criteria (fixtures/mail/README.md) — the behaviors the engine must exhibit, cited by the threading spec. The harness-only deps (nodemailer, imapflow) and package.json go with the harness; the engine's package.json comes with the engine. Helpthread's own e2e mail harness (pointing at a Helpthread instance) is future work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
The mail engine's acceptance criteria — the threading/conversation behaviors Helpthread must exhibit, captured as sanitized fixtures and reframed as our own acceptance suite (see
fixtures/mail/README.md), cited by the threading spec (HT-8).The FreeScout-observation harness that generated these was relocated to the private
resonant-helprepo (tools/mail-behavior-harness/, PR ResonantIQ/resonant-help#1) — it observes FreeScout specifically and serves no Helpthread deployer, so it doesn't ship in the distributed product. Helpthread's own e2e mail harness (pointing at a Helpthread instance) is future work.Provenance: black-box observation only (facts about behavior via API), token values + PII redacted to deterministic placeholders. No FreeScout source read/copied/derived.
Jira: https://resonantiq.atlassian.net/browse/HT-7
Summary by CodeRabbit
Documentation
Tests